Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support of non '.js' main #1291

Merged
merged 1 commit into from
Nov 22, 2015
Merged

Conversation

unional
Copy link
Contributor

@unional unional commented Nov 15, 2015

One custom step need is to add packages: { "myModule": { defaultExtension: "{myExtension, e.g. ts}" } }, which is ok to do before 0.17

@guybedford
Copy link
Member

Thanks. Did you manage to take a look at the failing test?

@unional
Copy link
Contributor Author

unional commented Nov 15, 2015

Looking at it.

@unional
Copy link
Contributor Author

unional commented Nov 15, 2015

It actually fail on main. It's not related to this PR. I checkout jspm/master and get same error.
Also validated the changed code is not being invoked in the failed test.

@unional
Copy link
Contributor Author

unional commented Nov 15, 2015

Really weird. Saw they pass on travis history, but I hard reset local to 0.16.14 (e5e9f27) and run test it still fail with same error.

@guybedford
Copy link
Member

@unional that would be due to caching. Run jspm cc and remove testlibs/jspm_packages before starting the tests.

@unional
Copy link
Contributor Author

unional commented Nov 15, 2015

Done!

@unional unional mentioned this pull request Nov 15, 2015
if (match) {
mainFileExtension = match[1];
main = main.slice(0, - mainFileExtension.length);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow why this is necessary? Surely line 783 can be left as-is with the same effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is checking for the lastNamePart. Could it has non .js there? If it can't, then this is redundant.

@unional
Copy link
Contributor Author

unional commented Nov 16, 2015

Coverage test failed with:
Would you like to set up your GitHub credentials?:Yes
Enter your GitHub username:undefined

non-issue?

return new Promise(function(resolve) {
mainPath = path.resolve(downloadDir, main + '.js');
mainPath = path.resolve(downloadDir, main + mainFileExtension);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be '.js'.

@guybedford
Copy link
Member

This looks good, thanks for getting it to this point!

Could you do a squash of these commits with a force push to clear up the history? Otherwise I can do it on merge no problem too.

@unional unional force-pushed the support-ts-package branch 2 times, most recently from c4979a3 to e52bb2a Compare November 19, 2015 17:22
@unional
Copy link
Contributor Author

unional commented Nov 19, 2015

Like this?

@guybedford
Copy link
Member

Looks like it caught some other stuff there, perhaps it needed a rebase as well?

@unional
Copy link
Contributor Author

unional commented Nov 19, 2015

Seems like the same error as the coverage failure last time. Is there a way to retry the build without committing?

@guybedford
Copy link
Member

I'm not so worried about that, but see the extra file contents at https://github.com/jspm/jspm-cli/pull/1291/files.

@unional
Copy link
Contributor Author

unional commented Nov 19, 2015

Oh....how come...
Let me check.

@unional
Copy link
Contributor Author

unional commented Nov 19, 2015

I did revert+amend to fix it. Seems good now. Is that the best way to do it? I still have lots of stuff to learn in using git.

And thanks for the careful code reviews. :)

guybedford added a commit that referenced this pull request Nov 22, 2015
@guybedford guybedford merged commit 2511c6d into jspm:master Nov 22, 2015
@guybedford
Copy link
Member

👍

@unional unional deleted the support-ts-package branch November 24, 2015 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants